Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add bookmode, zoom and dpi aware rendering to pdf player #4768

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add bookmode, zoom and dpi aware rendering to pdf player #4768

wants to merge 4 commits into from

Conversation

dulli
Copy link
Contributor

@dulli dulli commented Sep 2, 2023

After working on a plugin to read PDF metadata so that I could add magazines to Jellyfin, I realized that the PDF player is almost unusable on phones as you couldn't zoom and on the other hand it was basically hit or miss if the PDF was actually displayed in a readable way on desktop.

Changes

  • Improved the determination of the scale parameter to ensure non-blurry PDF rendering on both mobile and desktop viewports.
  • Added a book mode (i.e. displaying two pages each (except the cover and the last page) side by side on landscape viewports.
  • Added a way to zoom into the displayed PDF page(s) and scroll them.
  • Listen to click events (instead of touch events) to change the page, so that you actually can scroll the document without it directly changing the page (and also click through your PDF on the desktop without having to use the keyboard).

Issues

  • The blurry PDFs on high DPI displays were mentioned in Books (pdf) jellyfin-android#977 and allegedly fixed, but this introduced blurriness (for at least some?) PDFs on low DPI displays (i.e. desktop monitors). Now it shouldn't be blurry for both.

@dulli dulli requested a review from a team as a code owner September 2, 2023 18:23
@thornbill thornbill added enhancement Improve existing functionality or small fixes needs testing This PR requires additional testing labels Sep 8, 2023
@sonarcloud
Copy link

sonarcloud bot commented Sep 12, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
6.6% 6.6% Duplication

@dulli
Copy link
Contributor Author

dulli commented Dec 3, 2023

Is there anything I need to do / can do for this to progress?

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jan 12, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
1.2% Duplication on New Code

See analysis details on SonarCloud

@silentTeee
Copy link

This would solve a lot of issues for me! I have entire textbooks that I prefer to keep backed up on my server, but I like to reference from my phone occasionally.

As stated, the blurry text is still an issue for such PDFs, so it would really help to have this fixed!

@silentTeee
Copy link

Alright, I just played around with this by using the Preview URL and my own server, and I have to say the UX is much nicer:

On mobile (Firefox for Android)

Quality with PDFs

Current Release This PR
Screenshot_20240206-194237~2 Screenshot_20240206-194302

Zoom ability with PDFs

Current Release This PR
Can't zoom....trying will simply trigger a page change Screenshot_20240206-194308 Can zoom by pushing the expand button at the top-left, can scroll around without abruptly jumping to the next page

Page changing behavior

Current Release This PR
Tapping on left side brings to previous page, tapping on right side brings to next page. Tapping on left side brings to previous page, tapping on right side brings to next page.

Zooming via the button provided at the top-left does not break this

On PC (Firefox v 122.0 on Ubuntu 22.04 LTS)

Quality with PDFs

Current Release This PR
Screenshot from 2024-02-06 20-06-55 Screenshot from 2024-02-06 20-04-45

Zoom ability with PDFs

Current Release This PR
No built-in ability to zoom, the browser's zoom function works but does nothing to trigger increased quality as one zooms in (i.e. the initial render is all you get) Newly provided zoom button at top left works fairly well, resizing the two pages to the width of my screen.

One thing I did notice though is that the browser-provided zoom functionality does not work at all on my screen until you click the provided zoom button. Until you do that, the only thing that gets zoomed into is the UI-provided elements. But honestly that's just a nitpick.

Page changing behavior

Current Release This PR
Only the Arrow Keys can be used. Both the Arrow Keys and using the Mouse to click on the sides of the view can be used

I couldn't find any obvious bugs to speak of, either, so I'd say this is quite the winner of a PR!

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Sep 20, 2024
@jellyfin-bot

This comment has been minimized.

Sign in to view
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Sep 21, 2024
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Sep 21, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@jellyfin-bot
Copy link
Collaborator

Cloudflare Pages deployment

Latest commit a2304af87c66d8eaa145c2a2d3165d9fbb7a3814
Status ✅ Deployed!
Preview URL https://c5bcbc09.jellyfin-web.pages.dev
Type 🔀 Preview

@thornbill
Copy link
Member

Sorry this has taken forever to get feedback on @dulli. Could you possibly break each of the changes into separate PRs? Some of the new features should be implemented in the other book players as well and then we can prioritize getting the blur fix in asap. Thanks for your patience!

@kees-
Copy link

kees- commented Sep 24, 2024

These are great changes, I'll be pleased to see them implemented

@dulli dulli mentioned this pull request Oct 9, 2024
Merged
@jellyfin-bot
Copy link
Collaborator

This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Oct 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement Improve existing functionality or small fixes merge conflict Conflicts prevent merging needs testing This PR requires additional testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dulli @silentTeee @jellyfin-bot @thornbill @kees-